Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

argc: support cross-compilation #305729

Merged
merged 5 commits into from
May 4, 2024
Merged

argc: support cross-compilation #305729

merged 5 commits into from
May 4, 2024

Conversation

ShamrockLee
Copy link
Contributor

Description of changes

  • Move the Nix expression under name-based package directories.
  • Format the Nix expression with nixfmt in accordance with [RFC 0166] Nix formatting, take two rfcs#166.
  • Refactor the shell-completion-file-installation step and
    • Determine if host executables can be run on the build platform via stdenv.buildPlatform.canExecute stdenv.hostPlatform.
    • If so, add "${!outputBin}/bin" into the PATH used by installShellCompletion. (Such PATH value is named INSTALL_SHELL_FILE_PATH, so that we could generate and install man pages after argc 1.16).
    • If not, add argcNative into nativeBuildInputs and disallowedRequisites.
  • Add package test for cross-compilation.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Apr 21, 2024
@ofborg ofborg bot requested a review from figsoda April 21, 2024 13:17
@ShamrockLee ShamrockLee marked this pull request as ready for review April 22, 2024 04:50
@ShamrockLee
Copy link
Contributor Author

The Darwin cross-compilation test cases are affected by #273442 and fails to evaluate (infinite recursion). Here are the reason why I still add them:

  • OfBorg ignores evaluation failure of <pkg>.passthru.tests, and shows green as long as the package itself builds.
  • Potential fixes are being worked on. Darwin argc.tests should benefit from them.

@ShamrockLee ShamrockLee force-pushed the argc-cross branch 2 times, most recently from edfcb18 to 7a746be Compare April 26, 2024 19:06
@Mindavi
Copy link
Contributor

Mindavi commented Apr 28, 2024

Think this looks ok, but would like another pair of eyes

SuperSandro2000
SuperSandro2000 requested changes Apr 28, 2024
View reviewed changes
pkgs/by-name/ar/argc/package.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Show resolved Hide resolved
pkgs/by-name/ar/argc/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ar/argc/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ar/argc/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ar/argc/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ar/argc/package.nix Outdated Show resolved Hide resolved
@ShamrockLee
Copy link
Contributor Author

@SuperSandro2000 Some diffs mentioned in the reviews are caused by the formatter. Should I also refactor them in this PR?

@SuperSandro2000
Copy link
Member

Some diffs mentioned in the reviews are caused by the formatter. Should I also refactor them in this PR?

I would prefer to do less formatting changes to make the review easier but whatever you prefer.

@ShamrockLee
Copy link
Contributor Author

Some diffs mentioned in the reviews are caused by the formatter. Should I also refactor them in this PR?

I would prefer to do less formatting changes to make the review easier but whatever you prefer.

Next time I'll split out the formatting changes when working on similar features.

Given that the the formatted changes has been reviewed, I'm adding a commit to address them.

@ShamrockLee
argc: support cross-compilation
60949c8 
Use native argc to generate shell completion files
when "$out/bin/argc" cannot be executed on the build platform.
@ShamrockLee
argc: add passthru.tests.cross
c0eb246
@SuperSandro2000 SuperSandro2000 merged commit bc71d01 into NixOS:master May 4, 2024
24 checks passed
@ShamrockLee
Copy link
Contributor Author

@SuperSandro2000 Many thanks to you for reviewing this PR and polishing it!

@ShamrockLee ShamrockLee deleted the argc-cross branch May 4, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@figsoda figsoda Awaiting requested review from figsoda

Assignees
No one assigned
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ShamrockLee @Mindavi @SuperSandro2000